Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix metric mapping of counter metrics in the Openmetrics V2 version of the check #13573

Merged
merged 13 commits into from
Jan 12, 2023
Merged

Fix metric mapping of counter metrics in the Openmetrics V2 version of the check #13573

merged 13 commits into from
Jan 12, 2023

Conversation

steveny91
Copy link
Contributor

@steveny91 steveny91 commented Dec 23, 2022 •
edited
Loading

What does this PR do?

Counters weren't getting collected because Openmetrics v2 expects counter metrics to be specified without the _total. This PR removes _total from all the counter metrics in the openmetrics version of the check. This check never had a openmetrics v1 implementation. So I figured I can edit the main metric dictionary.

Also fixes one metric that was being mapped to the wrong datadog name envoy_cluster_upstream_cx_tx_bytes_buffered

@ghost ghost added the integration/envoy label Dec 23, 2022
@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

1 similar comment
@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@codecov
Copy link

codecov bot commented Dec 25, 2022 •
edited
Loading

Codecov Report

Merging #13573 (49ecebd) into master (4c20ed0) will not change coverage.
The diff coverage is n/a.

Flag Coverage Δ
envoy 94.23% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@steveny91 steveny91 marked this pull request as ready for review December 25, 2022 05:42
@steveny91 steveny91 requested a review from a team as a code owner December 25, 2022 05:42
alopezz
alopezz approved these changes Jan 3, 2023
View reviewed changes
Copy link
Contributor

@alopezz alopezz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have an answer to extra_metrics possibly not being honored (as mentioned in #12855 (comment))?

@steveny91
Copy link
Contributor Author

steveny91 commented Jan 5, 2023 •
edited
Loading

Do we have an answer to extra_metrics possibly not being honored (as mentioned in #12855 (comment))?

I tested it and I didn't have an issue adding an extra metric. For example:

# TYPE envoy_cluster_upstream_cx_http1_total counter
envoy_cluster_upstream_cx_http1_total{envoy_cluster_name="dummy_dynamic_cluster"} 0

with:

    extra_metrics:
      - envoy_cluster_upstream_cx_http1:
          name: test

I get:

$datadog-agent check envoy --table -r | grep test                            ok  7s  10:17:39 AM 
  envoy.test.count                                                         count  1672928266  0                     endpoint:http://localhost:8001/stats/prometheus, envoy_cluster:dummy_dynamic_cluster

I can't reproduce the issue.

@steveny91 steveny91 merged commit 0ea2110 into master Jan 12, 2023
@steveny91 steveny91 deleted the sy/envoy-omv2 branch January 12, 2023 14:55
@FlorentClarret FlorentClarret mentioned this pull request Jan 24, 2023
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alopezz alopezz alopezz approved these changes

Assignees
No one assigned
Labels
integration/envoy
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@steveny91 @alopezz